Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anypath abstractmethods #266

Closed

Conversation

chbehrens
Copy link
Contributor

Disclaimer: I quickly hacked this as a draft/suggestion. There might be better/nicer methods to achieve the same.

I suggest to add some abstractmethods to AnyPath so that the methods available for both CloudPath and pathlib.Path can be seen when AnyPath is used as type annotation. It would be nice if there are more elegant solutions but I think the overall behavior would be useful.

Currently missing:

  • Docstrings for functions that differ between pathlib.Path and CloudPath
  • CloudPathMeta could inherit from AnyPathMeta to avoid code duplication

I probably won't be around the next days but wanted to leave this as starting point to discuss.

@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2022

Codecov Report

Merging #266 (3d455b3) into master (98ed2e5) will decrease coverage by 2.4%.
The diff coverage is 73.2%.

@@           Coverage Diff            @@
##           master    #266     +/-   ##
========================================
- Coverage    94.8%   92.3%   -2.5%     
========================================
  Files          21      21             
  Lines        1321    1453    +132     
========================================
+ Hits         1253    1342     +89     
- Misses         68     111     +43     
Impacted Files Coverage Δ
cloudpathlib/anypath.py 76.1% <71.8%> (-23.9%) ⬇️
cloudpathlib/cloudpath.py 93.5% <100.0%> (ø)
cloudpathlib/s3/s3client.py 92.9% <0.0%> (-2.7%) ⬇️
cloudpathlib/gs/gsclient.py 92.8% <0.0%> (-1.8%) ⬇️

@chbehrens
Copy link
Contributor Author

chbehrens commented Sep 11, 2022

Any opinion on this @pjbull or @jayqi? I'm happy to make this a polished PR or go after another solution if you have any suggestions.

@pjbull
Copy link
Member

pjbull commented Sep 12, 2022

Sorry @chbehrens. Delay is because, as you point out, the current implementation is a lot of boilerplate which seems less than ideal just to support use of AnyPath as a type hint (does typing things as Union[CloudPath, Path] instead work for your purposes?). I don't have a better implementation off the top of my head, and like you say we should think about it in the context of our overall class hierarchy. Happy to open an issue for discussion, but I'm not super keen on this PR as is. Maybe @jayqi has a different opinion.

@chbehrens
Copy link
Contributor Author

Don't get me wrong, I'm also not keen on it the way it is. I just did it to show what I'd like to achieve in the end behavior-wise but preferably via something more meta programming like. That's why I made it a draft and was wondering whether you have some ideas or what you think of the overall intended behavior.
I'm sorry if it's the wrong format here, I just thought a draft PR might demonstrate it better than an issue.

@pjbull
Copy link
Member

pjbull commented Dec 18, 2022

@ringohoffman Do you have thoughts on this after implementing your changes in #298? Is there a cleaner/easier way to get useful type hinting for AnyPath without duplicating all the signatures?

@ringohoffman
Copy link
Contributor

I was asking about this over at microsoft/pyright#4331 and got back a few suggestions... and the virtual superclass approach was not one of them. I will have to play around to see which results in the most expected / convenient typing experience.

@pjbull
Copy link
Member

pjbull commented Dec 20, 2022

Much appreciated! Curious what you discover.

It does some interesting stuff in regard to typing.

😆 😆 😆

@pjbull
Copy link
Member

pjbull commented Jul 22, 2023

@chbehrens I'm going through old PRs to keep them moving.

I think that I've come around on this. I was hoping for some cleaner upstream fix that makes this kind of polymorphic class work, but I don't think there's another way to get useful completions for AnyPath until something like #347 comes in upstream in Python (and we can subclass from that).

Would you be willing to rebase this on the latest and add tests/docs?

Outside of that, I am curious if one of the other ideas in this thread might work: microsoft/pyright#4331 (comment)

It does seem that Protocol is potentially a friendlier implementation than ABC since it is slightly less boilerplate and plays nicer with static type checkers, but I'm not sure it solves all our issues without some experimentation.

@chbehrens
Copy link
Contributor Author

I'll try to have a look at it if I find time but let's just close this, an issue would be better suited to track the topic if necessary.

@chbehrens chbehrens closed this Jul 25, 2023
@jayqi
Copy link
Member

jayqi commented Jul 25, 2023

I've experimented with Protocols, and one of the main problems IIRC is that the Pydantic validation methods were getting caught up in it. There's also some annoying stuff with API changes to Path over different Python versions, and not being able to match all of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants